Skip to content

fix(cluster): surface named-task lookup failures as errors instead of silent partial results#42

Open
tablackburn wants to merge 3 commits into
mainfrom
fix/cluster-named-task-lookup
Open

fix(cluster): surface named-task lookup failures as errors instead of silent partial results#42
tablackburn wants to merge 3 commits into
mainfrom
fix/cluster-named-task-lookup

Conversation

@tablackburn
Copy link
Copy Markdown
Owner

@tablackburn tablackburn commented May 19, 2026

Summary

  • Get-StmClusteredScheduledTaskInfo was silently emitting partial results (TaskName-only) when a named task could not be resolved on a cluster.
  • Get-StmClusteredScheduledTask was firing a misleading "configure your cluster" warning for what was actually a single named-task miss.
  • Stage-B misses (cluster registry has the task but the owner node didn't return it) were leaking raw CmdletizationQuery_NotFound_TaskName errors to the console.

Changes

Get-StmClusteredScheduledTask.ps1

  • When -TaskName is given and Get-ClusteredScheduledTask returns zero, write a structured ClusteredScheduledTaskNotFound non-terminating error (ObjectNotFound category, TargetObject = $TaskName). Bulk path (no -TaskName) keeps the existing warning unchanged.
  • For each name not returned by the owner node's Get-ScheduledTask, write a structured ClusteredScheduledTaskOwnerLookupFailed error. Suppresses the raw cmdletization error leak.

Get-StmClusteredScheduledTaskInfo.ps1

  • When -TaskName is given and the inner lookup is empty, write a structured ClusteredScheduledTaskNotResolvable non-terminating error. Bulk path keeps the existing warning unchanged.

Behavior before vs after

Calling the user's loop pattern ($names | ForEach-Object { Get-StmClusteredScheduledTaskInfo -Cluster X -TaskName $_ }) with a mix of valid and invalid task names:

Before After
Invalid task 2 silent warnings, 0 errors, row TaskName-only 0 warnings, 2 attributed errors with FQEIs ClusteredScheduledTaskNotFound,Get-StmClusteredScheduledTask and ClusteredScheduledTaskNotResolvable,Get-StmClusteredScheduledTaskInfo
Stage-B miss Raw red CmdletizationQuery_NotFound_TaskName leak Structured ClusteredScheduledTaskOwnerLookupFailed error, no red leak
Valid task Full row Full row (unchanged)
Bulk call with empty cluster Warning Warning (unchanged)

Test plan

  • ./build.ps1 UnitTest — all 61 tests pass (5 new tests in this PR)
  • Lab-validated end-to-end against STMCLUSTER on the integration lab with AnyNode, ClusterWide, and an engineered stage-B miss
  • Backtick line-continuation scan on new/changed code — none introduced

Tests added

  1. Get-StmClusteredScheduledTask.When a named task is not found in the cluster (Issue 1 / stage-A miss).Should write a structured non-terminating error (not a warning) when -TaskName is given
  2. Get-StmClusteredScheduledTask.When a named task is not found in the cluster (Issue 1 / stage-A miss).Should still emit a warning (and no error) when -TaskName is omitted (bulk path)
  3. Get-StmClusteredScheduledTask.When the cluster claims an owner but the owner does not return the task (Issue 1 / stage-B miss).Should write a structured ClusteredScheduledTaskOwnerLookupFailed error
  4. Get-StmClusteredScheduledTaskInfo.When a named task cannot be resolved (Issues 1 + 2).Should write a structured non-terminating error (not a warning) when -TaskName is given
  5. Get-StmClusteredScheduledTaskInfo.When a named task cannot be resolved (Issues 1 + 2).Should still emit a warning (and no error) when -TaskName is omitted (bulk path)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Lookups for a named clustered task now emit structured, actionable errors (instead of only warnings) when the task cannot be resolved, offering clearer troubleshooting guidance.
    • Validation of task ownership now reports structured errors when expected tasks are missing from an owner, replacing prior warning-only behavior to surface lookup failures.
  • Tests

    • Test coverage updated to assert the new structured error and preserved warning behaviors for bulk vs named lookup scenarios.

Review Change Stack

… silent partial results

Previously, Get-StmClusteredScheduledTaskInfo silently emitted partial results
(TaskName-only) when a named task could not be resolved on a cluster, and
Get-StmClusteredScheduledTask fired a misleading "configure your cluster"
warning for what was actually a single-named-task miss.

Three changes across two files:

- Get-StmClusteredScheduledTask.ps1: when -TaskName is given and the cluster
  returns zero, write a structured ClusteredScheduledTaskNotFound error instead
  of a cluster-wide warning. Bulk path (no -TaskName) keeps the warning.
- Get-StmClusteredScheduledTask.ps1: detect stage-B misses (cluster registry
  has the task but the owner node didn't return it) and write a structured
  ClusteredScheduledTaskOwnerLookupFailed error per missing task. Suppresses
  the raw cmdletization error leak from the per-name Get-ScheduledTask call.
- Get-StmClusteredScheduledTaskInfo.ps1: when -TaskName is given and the inner
  lookup is empty, write a structured ClusteredScheduledTaskNotResolvable
  error. Bulk path keeps the warning.

Five new Pester tests across the two .Tests.ps1 files cover both error paths
and verify the preserved bulk-path warnings. Validated red-then-green:
new tests fail against pristine module, pass with patches. Also validated
end-to-end against a real failover cluster in the integration lab.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 19, 2026 01:06
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Warning

Rate limit exceeded

@tablackburn has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 34 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7ff5bf12-83ba-4214-8e1d-95a97b41c77e

📥 Commits

Reviewing files that changed from the base of the PR and between c46e052 and ccda643.

📒 Files selected for processing (1)
  • ScheduledTasksManager/Public/Disable-StmClusteredScheduledTask.ps1
📝 Walkthrough

Walkthrough

This PR makes named-task misses produce structured ObjectNotFound errors (with ErrorId/RecommendedAction) and adds explicit owner-lookup mismatch errors; bulk queries keep warning behavior. Tests and integration calls updated to cover and tolerate the new behaviors.

Changes

Clustered task query error handling

Layer / File(s) Summary
Get-StmClusteredScheduledTask error handling and tests
ScheduledTasksManager/Public/Get-StmClusteredScheduledTask.ps1, tests/Get-StmClusteredScheduledTask.Tests.ps1, tests/Integration/ClusteredScheduledTask.Integration.Tests.ps1
When no clustered tasks are found, the cmdlet emits a structured ObjectNotFound error if -TaskName was provided; otherwise it warns. Per-owner retrieval suppresses cmdlet errors, reconciles expected vs returned task names, and emits ClusteredScheduledTaskOwnerLookupFailed for missing names. Tests assert named vs bulk behavior and owner-lookup failures; integration calls use -ErrorAction Ignore for cleanup checks.
Get-StmClusteredScheduledTaskInfo error handling and tests
ScheduledTasksManager/Public/Get-StmClusteredScheduledTaskInfo.ps1, tests/Get-StmClusteredScheduledTaskInfo.Tests.ps1
Treats both $null and empty clustered-task results as “not found.” When -TaskName is specified, emits a structured ClusteredScheduledTaskNotResolvable (ObjectNotFound) with a RecommendedAction; bulk queries preserve warning behavior. Tests verify named error and bulk warnings.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

A rabbit hops through error paths with care,
Structured messages float through the air—
No task found? An error declares it clear,
With guidance and wisdom for those who'll hear! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main objective: converting silent/misleading partial results into structured errors for named-task lookup failures.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cluster-named-task-lookup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR replaces silent/misleading behavior in Get-StmClusteredScheduledTask and Get-StmClusteredScheduledTaskInfo with structured non-terminating errors when a specifically named clustered task cannot be resolved, while preserving the existing warning behavior on bulk (no -TaskName) calls. It also suppresses the raw cmdletization error leak from Get-ScheduledTask and re-emits a structured ClusteredScheduledTaskOwnerLookupFailed error per missing task.

Changes:

  • Emit ClusteredScheduledTaskNotFound (stage-A) and ClusteredScheduledTaskOwnerLookupFailed (stage-B) errors from Get-StmClusteredScheduledTask for named-task misses.
  • Emit ClusteredScheduledTaskNotResolvable from Get-StmClusteredScheduledTaskInfo when a named lookup returns nothing.
  • Add 5 Pester tests covering named/bulk paths and the stage-B miss.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
ScheduledTasksManager/Public/Get-StmClusteredScheduledTask.ps1 Adds structured errors for stage-A (task name not in cluster registry) and stage-B (owner node didn't return registered task) misses; bulk path warning preserved.
ScheduledTasksManager/Public/Get-StmClusteredScheduledTaskInfo.ps1 Adds structured ClusteredScheduledTaskNotResolvable error when -TaskName is supplied but inner lookup is empty; bulk path warning preserved.
tests/Get-StmClusteredScheduledTask.Tests.ps1 New tests for stage-A error, bulk warning preservation, and stage-B owner-lookup error.
tests/Get-StmClusteredScheduledTaskInfo.Tests.ps1 New tests for the named-task error and bulk warning preservation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

… contract

Drop the stale "Should warn" unit test — its scenario is now covered by the
PR's new structured-error tests (`ClusteredScheduledTaskOwnerLookupFailed`),
and its old warning-based assertion no longer matches the post-#42 behavior.

Switch four integration-test cleanup probes from `-EA SilentlyContinue` to
`-EA Ignore`. `Ignore` does not pollute `$Error`; `SilentlyContinue` does.
The polluted `$Error` was leaking across the PSRemoting boundary into
psake's `$ErrorActionPreference = 'Stop'` scope and failing the build at
`build.psake.ps1:266` after all 19 integration tests had passed.

Probe-then-act pattern is preserved (Unregister-* is still terminating on
missing tasks); collapsing to one-line idempotent cleanup is tracked in #44.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…ify step

Disable-StmClusteredScheduledTask's verify-after-unregister called
Get-StmClusteredScheduledTask with -ErrorAction Stop, expecting a silent
null when the task was successfully unregistered. After this PR introduced
the new ClusteredScheduledTaskNotFound error on the named-task path, that
Get call now throws terminating on the SUCCESS case (task absent), which
the surrounding catch block wraps as UnregisterFailed and re-throws.

In the integration test, the "Should disable" Pester test still reported
as passed (AutomatedLab's Invoke-LabCommand appears to swallow terminating
errors from the remote lab session), but the error accumulated in the
session's error stream and surfaced ~2s after Pester finished, at the
outer Invoke-Command boundary in build.psake.ps1:266 where psake's
$ErrorActionPreference = 'Stop' promoted it to a build failure.

Empirically verified locally: Get-StmClusteredScheduledTask with the exact
splat Disable used (-EA Stop, -WA SilentlyContinue) throws terminating
with FQEI ClusteredScheduledTaskNotFound when the task is missing.

Switch to -ErrorAction Ignore on the verify-Get: a missing task here is
the SUCCESS condition. The existing `$taskExists = $null -ne $task` check
correctly drives the success/failure branch without depending on the
producer's error contract.

Audit of other internal Get-StmClusteredScheduledTask callers:
- Import-StmClusteredScheduledTask: -EA Stop, but wraps in try/catch that
  treats throw as 'doesn't exist'. Robust to both old and new behavior.
- Enable/Export/Set/Start/Stop/Wait/Get-Info: all expect the task to
  exist; surfacing 'not found' as an error is the correct behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants